Skip to content

Add type annotations to manim/_config/utils.py #4230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Add type annotations to manim/_config/utils.py #4230

wants to merge 14 commits into from

Conversation

henrikmidtiby
Copy link
Contributor

@henrikmidtiby henrikmidtiby commented Apr 27, 2025 •
edited by chopan050
Loading

Overview: What does this pull request change?

More work on #3375

Continuation of PR #4134

The module manim/_config/utils.py is now going through mypy checks without raising any issues.

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

@github-project-automation github-project-automation bot moved this to 🆕 New in Dev Board Apr 27, 2025
@henrikmidtiby henrikmidtiby mentioned this pull request Apr 27, 2025
Merged
3 tasks
@henrikmidtiby
Copy link
Contributor Author

@JasonGrace2282 Would you take a look at this PR?

@henrikmidtiby henrikmidtiby force-pushed the typing-logger-utils branch from 5484d9e to 3dd725d Compare May 23, 2025 07:04
behackl
behackl approved these changes Aug 11, 2025
View reviewed changes
Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me, thank you very much!

@github-project-automation github-project-automation bot moved this from 🆕 New to 👍 To be merged in Dev Board Aug 11, 2025
@behackl behackl enabled auto-merge (squash) August 11, 2025 17:14
@behackl behackl added this to the v0.19.1 milestone Aug 11, 2025
@behackl behackl added the typehints For adding/discussing typehints label Aug 11, 2025
behackl
behackl requested changes Aug 11, 2025
View reviewed changes
Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon closer inspection (and actually due to our pre-commit mypy check): there now is an issue with the config.window_size key typing that revealed a bug:

The OpenGLWindow does expect config.window_size to be a string of shape 'width,height', or 'default'. If one sets config.window_size = (900, 600) or something like that (which is actually quite reasonable to assume, that an integer-tuple is a suitable choice), then the window crashes because it tries to call .split on the config value.

Suggestion (up for discussion of course): we could rewrite the setter of config.window_size to be tuple[int, int] | None, with None taking over the current role of 'default', and then simplify the logic in opengl_renderer_window.py. Thoughts?

@github-project-automation github-project-automation bot moved this from 👍 To be merged to 👀 In review in Dev Board Aug 11, 2025
@chopan050
Copy link
Contributor

Since we merged PR #4363 which typed opengl_renderer_window.py and stopped ignoring its errors, there was a new error raising in there because of the type of the window_size parameter of Window. I made a commit which fixes that error by allowing window_size to be a tuple of ints and further checking its values.

@chopan050 chopan050 changed the title Typing logger utils - continuation of PR #4134 Add type annotations to manim/_config/utils.py Aug 11, 2025
@chopan050
Copy link
Contributor

chopan050 commented Aug 11, 2025 •
edited
Loading

I just saw @behackl comment. Before reading it, I already committed a fix 😅

Suggestion (up for discussion of course): we could rewrite the setter of config.window_size to be tuple[int, int] | None, with None taking over the current role of 'default', and then simplify the logic in opengl_renderer_window.py. Thoughts?

Would that also make the getter of config.window_size return tuple[int, int] | None? If so, that would be a nice change. I like the idea.

Actually, instead of having too much logic for validating and parsing window_size in Window, there should probably also be a parsing logic inside config.window_size's setter.

EDIT: this should all probably be done in a follow-up PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behackl behackl behackl requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
typehints For adding/discussing typehints
Projects
Dev Board
Status: 👀 In review
Milestone
v0.19.1
Development

Successfully merging this pull request may close these issues.
4 participants
@henrikmidtiby @chopan050 @behackl @fmuenkel